feat: enhance agent runner - #404

✅ No new issues found.

Reviewed changes — Full initial review of all 23 files in commit bca8214 (agent-runner, apps/mcp, apps/web): read the entire diff front to back, traced every changed seam against the surrounding code, verified the third-party Goose provider claims against block/goose source, and ran the touched Go packages' unit tests (all pass under -race).

• Per-conversation trigger serialization — a refcounted per-conversation_id lock in the Valkey consumer guarantees two triggers for the same conversation never run Handler concurrently, closing the in-memory event_index increment and in-flight-registry races at their root; lock is acquired before the semaphore slot so queued triggers don't exhaust it.
• Token-owned in-flight registrations — Register/Unregister now hand back an ownership token so a stale deferred Unregister can't clear a newer turn's entry (pause/resume turn overlap case); TeardownPausedChatSandbox re-checks IsRegistered before popping so the idle reaper or a stop can't tear a resuming turn's sandbox out from under it.
• ACP bridge reconnect leak fix — Register now cancels, closes, and waits on a superseded same-process connection, so its forwarder/eviction-watcher goroutines and Redis Pub/Sub subscription actually exit instead of running forever against an orphaned connection.
• Bundled-skills failure surfaced as terminal — the conversation is marked running before skill load, so a transient services/api outage during BundledSkills.Load lands as a visible failed status + ack instead of an unacked message that is never actually redelivered (no XCLAIM in this consumer).
• Goose provider ID aliases — gemini→google and deepseek→custom_deepseek via an explicit alias table; verified against block/goose source (GOOGLE_PROVIDER_NAME, deepseek.json). Both previously failed to initialize silently for every conversation on those providers.
• Global-chat and automation prompt context — nil-project triggers get a global-agent context block instead of a bogus nil-UUID framing; automation-fired triggers get the "no human watching" note; PACA_ACTOR_USER_ID is now sourced from ActorUserID (was ActorMemberID, a member id mislabeled as a user id) so global-chat identity reaches apps/mcp.
• Turn-scoped diff baseline — a per-path baseline lets a second edit of the same file in one turn diff incrementally against the first edit's result rather than always against HEAD.
• Sandbox image-confirmation cache — ensureImage skips ImageList/ImagePull entirely for an already-confirmed pinned ref on subsequent sandbox starts.
• Realtime tail-cache pruning — useConversationEventWindow prunes the tail buffer down to events past the fetched window once a real fetch covers them, bounding a previously unbounded per-tab growth.
• clone_repository delete guard — assertSafeDeleteTarget refuses the recursive rm against a hand-picked set of top-level system directories regardless of what path the agent supplies, closing a prompt-injection crater the Python port it replaces never had.
• Docs — new services/agent-runner/README.md and expanded architecture doc, including the already-wired PACA_MCP_DEV_SOURCE_DIR.

Every load-bearing claim in the doc comments checks out against the code and — where it matters (provider IDs) — against upstream Goose source. The concurrency fixes are subtle and each carries a regression test that genuinely fails without the fix. Nothing to block or change.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

✅ No new issues found.

Reviewed changes — Incremental delta since the prior review (commit bca8214): the new commit 479e0ca ("fix: resolve CI lint failures") is purely mechanical formatting — gofmt field alignment in convlock.go, goimports grouping in prompt.go, and a biome import-order fix in use-conversation-event-window.ts. No behavioral surface.

The original substantive review stands; nothing in this commit changes any of the previously-reviewed behavior.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

ℹ️ No critical issues — the new commit is correct and well-tested. One minor consolidation suggestion inline.

Reviewed changes — Incremental delta since the prior review (commit 479e0ca): commit ab390b0 ("feat: implement per-key locking mechanism and enhance concurrency safety in agent runner").

• Per-agent registerLocks in the ACP bridge — Register swapped the single process-wide registerMu for a convlock Locks keyed per agent_id, so one agent's potentially unbounded eviction wait (<-prev.done) can no longer stall every other agent's Register on this replica. Correct: presence, connection-map, and broadcast state are all strictly per-agent. Verifying test TestRegister_DifferentAgentsDoNotBlockOnEachOthersEviction genuinely blocks one agent's connector and asserts another's Register completes.
• resumeLock closes the Handle-vs-Teardown check-then-act race — Handle's registrar-of-flight + ChatSandboxes.Get now runs atomically, per conversation_id, with TeardownPausedChatSandbox's IsRegistered-check + Pop, so a stop/reaper can't tear down the sandbox a resuming turn just read. Held only across the two fast in-memory ops, never for the whole turn. The race test uses real registry.Conversations + chatsandbox.Registry and would fail without the lock.
• New generic internal/convlock package — per-key refcounted mutex used by both the handler and the bridge (see inline note).
• clone_repository hardening — /home/goose (the container user's home, parent of the default clone dir) added to the forbidden-delete set, with traversal test cases (/home/goose/../ → /home).

Verified: go build ./..., go vet, and go test -race all clean on the touched packages (convlock, handler, acpbridge, messaging). Nothing to block.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

✅ No new issues found.

Reviewed changes — Incremental delta since the prior review (commit ab390b0): commit d641507 ("refactor: replace conversationLocks with convlock for improved trigger serialization") consolidates the duplicated per-key refcounted mutex.

• Deduplicated the per-key lock primitive — deleted internal/messaging/convlock.go (conversationLocks) and switched the messaging.Consumer to the shared convlock.Locks package (convlock.New() / Lock(trigger.ConversationID)), with doc comments updated to match. This directly implements the consolidation suggestion from the prior review.
• Behavior-preserving — convlock.Locks is byte-for-byte the same refcounted mutex conversationLocks was; the per-conversation_id serialization (lock acquired before the semaphore slot) and the regression test are unchanged. Verified the lock is still held only across the semaphore-acquire and handler call, never the whole run loop.

Verified locally: no lingering references to the old symbols, go build ./... and go vet ./... clean, and go test -race ./internal/messaging/... ./internal/convlock/... pass, including TestConsumer_SerializesTriggersForTheSameConversation. Consolidation done as suggested; nothing to block.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

ℹ️ No critical issues — the new preset instructions are well-formed and their ## Skill: … anchors genuinely resolve below the system prompt at runtime; two minor prompt-model notes inline.

Reviewed changes — Incremental delta since the prior Pullfrog review (commit d641507): the new commit 1eb96dee (single file, apps/web/src/lib/agent-api.ts). Each bundled LLM preset's defaultSystemPrompt now appends: "when you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the <preset> skill (## Skill: <name> below)" — software-engineer → paca-do, code-reviewer & qa-engineer → paca-test, planner → paca-sprint, business-analyst → paca-clarify.

• Prompt text, no code changes — the create-agent dialog stores these strings as the agent's system_prompt, which agent-runner's buildInitialMessage emits first, followed by every enabled bundled skill as ## Skill: <name> sections. Confirmed against services/agent-runner/internal/executor/prompt.go:83-96 and services/api/internal/platform/bundledskills — all four referenced skills exist as agent-flavor entries (none CLIOnly), so nothing dangles.
• Consistent injection for every LLM conversation — the handler always prepends the bundled agent-flavor set to cfg.Skills (handler.go:135-152), so the referenced sections are always present below the system prompt regardless of per-agent skill customization.

Two optional refinements inline (both non-blocking observations, not defects).

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

ℹ️ No critical issues — the skill-delivery rework and async-ACP client are thorough, well-documented, and genuinely tested (go test -race clean on the touched packages). One minor resource-lifecycle note inline.

Reviewed changes — Incremental delta since the prior Pullfrog review (commit 1eb96dee): commit a2a16dd1 ("feat: add tests for skill handling and improve skill delivery via Goose").

• Moved skill delivery to Goose's native file mechanism — every enabled skill (custom ones with no frontmatter included) is now written to the sandbox as a real SKILL.md under .agents/skills/ via a tar (executor/skills.go) and discovered through load_skill, instead of being folded into the first-turn prompt; the agent's own system prompt plus a mandatory load_skill(paca) bootstrap now ride a .goosehints file (executor/hints.go).
• Rewrote the ACP client for Goose 1.46's async transport — POSTs get a bare 202, responses arrive on connection/session-scoped SSE streams correlated by JSON-RPC id, with background reader goroutines torn down via Client.Close(); session/new MCP servers + the skills extension ship through _meta.enabledExtensions with stdio env as names only.
• Bumped the sandbox image to ghcr.io/aaif-goose/goose@sha256:3c961bac… (1.46.0, the first that ships the skills feature), added sandbox.CopyToContainer, and wired Client.Close() into both tearDownSandbox and TeardownPausedChatSandbox.
• Updated prompts/presets — the bundled paca skill and the web preset system prompts now route via load_skill by name rather than referencing inlined ## Skill: sections.
• Tests — frontmatter/tar/hints/async-client unit tests plus a real-Docker e2e test (gated on PACA_E2E=1) asserting the system prompt reaches the system role and no skill body is ever folded into a message.

Verified go build, go test, and go test -race clean on internal/acp, internal/executor, internal/handler, and internal/sandbox.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

Reviewed changes — Incremental delta since the prior pullfrog review (commit a2a16dd): commit d3318f4 ("feat(agent): implement Docker-in-Docker sidecar for per-conversation isolation and access").

• Added a per-conversation docker:27-dind sidecar (internal/sandbox/dind.go) on a private bridge network only the paired sandbox container joins, with the sandbox's DOCKER_HOST pointed at the sidecar by deterministic name over plaintext port 2375.
• Wired sidecar lifecycle into sandbox.Start/Stop — the sidecar is created before the sandbox's own image/container (so DOCKER_HOST is in place from the first line), torn down on every failure path via the sidecarOK defer, and force-NetworkDisconnected before network teardown in Stop.
• Switched the agent-server image to run as root and installed the docker CLI (services/agent-server/Dockerfile).
• Added e2e coverage: root package-manager ops + docker run hello-world, and per-conversation sidecar isolation; plus unit tests for the deterministic naming helpers.
• Deploy config (docker-compose.dev/prod) mounts the Docker socket and documents the dedicated-host security posture.

ℹ️ Orphaned privileged Dind sidecars and networks on process death

stopDindSidecar and NetworkDisconnect only run in-process inside Sandbox.Stop. If the agent-runner process dies abruptly (crash, SIGKILL, pod eviction), every conversation's sidecar — a privileged container holding 2 CPU/4 GiB, plus its private network — is left running indefinitely on the host with no startup sweep of paca.managed artifacts. Because dindContainerName is deterministic, a later start for the same conversation would then also hit a container-name conflict with the orphaned sidecar. Worth adding a best-effort startup reaper that force-removes orphaned labelManaged containers/networks.

# Orphaned protected Dind sidecars on process death

## Affected sites
- services/agent-runner/internal/sandbox/dind.go:192-201 — stopDindSidecar (only called from Stop / Start-failure defer)
- services/agent-runner/internal/sandbox/sandbox.go:390-406 — teardown in Stop

## Required outcome
- A crash/restart should not leave privileged dind sidecars and their networks running forever, and must not make later same-conversation starts fail with a container-name conflict.

## Suggested approach
- On Manager construction (or service startup), list containers/networks by `labelManaged=true` and force-remove orphans whose owning conversation is no longer active.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

ℹ️ The new commit is clean — no new issues in this delta. Not approving only because the PR still carries the open [!IMPORTANT] containerIP must-fix from the d3318f4 review, which this commit doesn't touch.

Reviewed changes — Incremental delta since the prior review (commit d3318f4): commit 8922c7e ("feat(agent): add general-purpose developer tools to Docker image for enhanced task execution"), a single-file change to services/agent-server/Dockerfile.

• Added a general-purpose developer-tool layer — a new apt layer installs ripgrep, tree, jq, unzip, zip, less, nano, sqlite3, openssh-client, python3-pip, build-essential, postgresql-client, and shellcheck into the sandbox image so a conversation doesn't fall back on slow workarounds or apt-get install mid-task. Closed with apt-get autoremove and /var/lib/apt/lists/* cleanup.
• Kept curl (previously purged like gnupg) since it's now itself one of the general-purpose tools.
• Documented rationale — the layer is thoroughly commented: rg/tree close a real gap vs Goose's own default system prompt, --no-install-recommends avoids pulling in python3-dev for python3-pip, and the +106MB measured size (build-essential being most of it) stays within the layer's stated 256MB budget.

The change is a well-documented, size-measured image build addition with no behavioral or correctness issues to raise. It leaves the three open review threads from prior commits untouched (containerIP must-fix, coldStart connStream leak, orphaned-sidecar concern); the containerIP thread in particular still blocks merge.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

ℹ️ All four prior review threads are resolved and the delta builds/vets clean — no new issues. One minor test-coverage gap inline.

Reviewed changes — Incremental delta since the prior Pullfrog review (8922c7e): commit 9cba883 (formatting-only, acknowledged via progress) and commit b2d0bc36.

• Preset prompts now explicitly override paca routing — every bundled preset's defaultSystemPrompt in agent-api.ts inserts the clause "this overrides the paca skill's own status-based routing table", and the planner preset scopes paca-sprint to "a specific existing task (as opposed to a goal to break down from scratch)". Resolves both open preset threads.
• coldStart closes the ACP client on NewSession failure — client.Close() is now called (executor.go:302) before returning nil, tearing down the connection-scoped SSE reader started by the successful Initialize instead of leaking it. This implements the previously-flagged fix exactly. Resolves the open connStream-leak thread.
• Deterministic containerIP network selection — containerIP gained a preferredNetwork parameter; the insideDocker branch passes ownNetName, so the sole waitForReady candidate is always the process-reachable own-network IP rather than a coin-flip between the sandbox's two networks (with the non-containerized branch passing "" to keep the localhost fallback). Directly implements the blocking must-fix. Resolves that thread.

Verified go build ./... and go vet ./internal/sandbox/... ./internal/executor/... clean locally.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

✅ No new issues found.

Reviewed changes — Incremental delta since the prior Pullfrog review (b2d0bc3, review #4949294029): commit 4ebba70f ("feat(agent): enhance container IP selection logic and add comprehensive unit tests").

• selectContainerIP extracted as a pure function + regression tests — containerIP's network-selection rule is pulled out of the ContainerInspect call into a standalone selectContainerIP(networks, preferredNetwork) (string, bool), then covered by four unit tests (preferred-network selection, absent-preferred fallback, empty-preferred acceptance, and no-valid-address → false). This directly closes the minor test-coverage gap from the prior review: the previously-critical containerIP coin-flip fix now has real lock-in, exercised without needing a Docker daemon.

Verified locally: go test ./internal/sandbox/... passes, including the four new selectContainerIP tests. The prior thread (sandbox.go:591) is resolved. The refactor is behavior-preserving and well-documented; nothing to block.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}